Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nightwatch flags inspection during installation #101

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samson-olawoyin
Copy link

This feature inspect the inputed flags (-- --mobile or -- --generate-config) when installing Nightwatch and then give suggestion on invalid flags detection and then terminate Nightwatch installation setup.

@samson-olawoyin samson-olawoyin changed the title Nightwatch flags inspect during installation Nightwatch flags inspection during installation Mar 7, 2024
@samson-olawoyin
Copy link
Author

@garg3133 Trust you are doing very well today, this is a PR for Nightwatch issue #85, please help review when you have some moment.

src/index.ts Outdated

//Check the passed flags when installing Nightwatch and then suggest when undefined flag is been found.
if (argv[0] !== undefined) {
if ((argv[0] !== '--mobile') && (String(argv[0]) !== '--generate-config')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olawoyinsamson You should use the options variable here instead of directly using argv. See minimist documentation for the difference between args and options here.

Also, your implementation is wrong, if you try running the tool locally with npm run create-nightwatch -- --mobile --generate-config, it will exit even when the flags passed are correct. Plus, you should not check for the flags like this (think in the lines of if we want to add more flags in the tool, how should we do it; we shouldn't need to make changes at multiple places throughout the code while doing so).

An implementation of what we're trying to achieve (give suggestions in case wrong flags are passed) is already present below in this file itself, so you should ideally make changes to that to use didYouMean instead of suggestSimilarOption, and exit the process in case wrong flags (or even arguments) are passed to the tool.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garg3133 Thank you for your review, i have now re-implemented the issue #85 fix as you suggested above, kindly help review again when you have a moment.

Signed-off-by: Samson Olawoyin <olawoyin.samson@outlook.com>
@@ -7,7 +7,7 @@ import {NightwatchInitiator} from '@nightwatch/setup-tools';
import {NIGHTWATCH_TITLE, AVAILABLE_CONFIG_FLAGS} from './constants';
import Logger from './logger';
import minimist from 'minimist';
import suggestSimilarOption from './utils/suggestSimilar';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove ./utils/suggestSimilar. Also, what's the motive behind this change?
Also, why not use didyoumean or didyoumean2?

Copy link
Author

@samson-olawoyin samson-olawoyin Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavsingh97 thank you for your review, it was suggested that didyoumean library be used in place of the module suggestSimilarOption for suggesting Nightwatch flags when a wrong flags is been detected and then terminate the Nightwatch process. The Nightwatch issue is: #85. I will refactor the changes now with didyoumean2 because didyoumean do not support module implementation and then remove the suggestSimilarOption.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaibhavsingh97 i trust you are doing very well today, i wanted to mentioned that i have updated this PR with your suggestions above and now the PR has also passed the required check but could you please help look at this when you have some moment.

Signed-off-by: Samson Olawoyin <olawoyin.samson@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants